Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

Trace tool #1723

Closed
wants to merge 7 commits into from
Closed

Trace tool #1723

wants to merge 7 commits into from

Conversation

ndmlny-qs
Copy link
Contributor

Motivation

Continued work on the diagnostics tool, this includes a model trace tool.

Changes proposed

  • Changes include new JavaScript and Python files for the Bokeh Application.
  • Updates to helper modules in stats for the JavaScript

Test Plan

The tool was run in the Coin flipping tutorial.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

@ndmlny-qs ndmlny-qs self-assigned this Sep 30, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 30, 2022
@ndmlny-qs
Copy link
Contributor Author

@horizon-blue I am working on updating the notebook to mdx converter based on the revisions in this PR. I think this tool would be the first to merge as it directly updates (no change to prose) the coin flipping tutorial by adding it to the tutorial.

@horizon-blue
Copy link
Contributor

:) sounds good. Internally I got Yarn and webpack successfully set up for #1691, though I can't merge my config yet because #1691 is still WIP. If we think this one will be merged sooner, then I will rebase my internal changes to this PR.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ndmlny-qs
Copy link
Contributor Author

@horizon-blue that's spectacular! I don't have a use case for PR #1691 yet, but do in this one which is why I suggested merging this first. But, I think if you can merge #1691 then I'll remove the [WIP] flag, and I can keep building from there.

@horizon-blue
Copy link
Contributor

:) Finally merged #1691 after it pass the internal tests. However, it looks like we have to resolve quite a bit of merge conflicts before I can test out this PR.

Also, just as a FYI, I changed the diagnostic accessor a bit. It now lazily imports the widgets only when necessary. That way we don't have to add Bokeh and IPython to our core dependencies.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@horizon-blue
Copy link
Contributor

I took the liberty to resolve the merge conflicts with the marginal 1D tool so I can show both widgets in a single notebook. Hopefully I didn't break anything during the merge (the widget looks fine on my local jupyter session, though I still need to try it out to tell if it works in Bento.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@feynmanliang
Copy link
Contributor

fbcode/beanmachine/beanmachine/ppl/diagnostics/tools/js/src/stats/histogram.ts
Warning
Line 1
LICENSELINT • missing-license
Hide Details
Open in VS Code @ FB
More info: https://fburl.com/fbsource-linters#license-lint

@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@ndmlny-qs ndmlny-qs changed the title [WIP] Trace tool Trace tool Oct 19, 2022
@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@horizon-blue
Copy link
Contributor

Looks like we need to resolve the merge conflict on the Coin flipping tutorial again :). Can we also make sure that it's up-to-date? Looks like there're some changes in the trace plot widget that's not reflected in the tutorial.

ndmlny-qs and others added 7 commits October 23, 2022 19:09
This commit includes the marginal 1D diagnostic tool with JavaScript
callbacks.
- Removes ArviZ as a dependency from the diagnostic tool.
- Adds jsdoc Documentation for the TypeScript methods.
- Fixes flake8 linting error.
- Adds clarifying comments to the tool.
- Adds missing copyright notice.
- Aligns type names between Python and TypeScript. There is now a
  one-to-one correspondence between Python TypedDict objects and the
  TypeScript interface objects.
- Redundancy in the Python TypedDict objects has been removed.
- Fixes tooltips not rendering correctly.
- Updates the link in the Help Tab of the tool such that when clicked it
  opens a new tab in the browser.
- Moves a few peerDependencies to the devDependencies section in the
  package.json file. This was done because yarn issued a warning
  indicating there were missing peer dependencies.
- Refactors the tool such that no calculations for the data displayed is
  done in Python. All updates and all calculations for the tool data is
  done exclusively in the browser. The rational for this change was to
  prevent users from having initial data calculated by Python, and
  subsequent updates calculated by JavaScript in the browser when
  interacting with the tool. There are notes in the code indicating why
  this change was made.
This tool branches from the marginal1d tool and rebases against main.
Added files include those needed for displaying a Trace diagnostic tool
for Bean Machine models.

This also updates the Coin_flipping.ipynb tutorial to include the new
diagnostic tool.
- Rebased against `main`, and fixed merged conflicts.
- Updated copyright requirement
- Updated how models are serialized.
- Update style based on other pushes to the branch
- Added `__future__` annotations
- updated a few docstrings that needed an update
- updated yarn lock based on changes with the marginal1d tool and the
  trace tool.
- removed the 3rd party histogram requirement, since it is easy to
  calculate a histogram.
- other fixes associated with merging etc.
@facebook-github-bot
Copy link
Contributor

@ndmlny-qs has updated the pull request. You must reimport the pull request before landing.

@ndmlny-qs
Copy link
Contributor Author

I rebased, fixed merge conflicts, and made sure coin flipping tutorial was the one already in the main branch. Let me know if there are any other problems I should address with this PR.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ndmlny-qs ndmlny-qs deleted the trace-tool branch October 25, 2022 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants